Skip to content

refactor: extract AppCrashLogger from App.xaml.cs - #607

Merged
ranjeshj merged 1 commit into
openclaw:masterfrom
AlexAlves87:refactor/extract-crash-logger
Jun 2, 2026
Merged

refactor: extract AppCrashLogger from App.xaml.cs#607
ranjeshj merged 1 commit into
openclaw:masterfrom
AlexAlves87:refactor/extract-crash-logger

Conversation

@AlexAlves87

@AlexAlves87 AlexAlves87 commented May 30, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Summary

Extract the crash-logging sink out of App.xaml.cs into a small dedicated
AppCrashLogger, mirroring the existing AppRunMarker. Pure refactor, no
behavior change.

What changed

  • New Services/AppCrashLogger.cs: takes the crash-log path in its constructor
    and exposes Log(string source, Exception? ex) — the exact body of the former
    static LogCrash (append to crash.log + Logger.Error).
  • App.xaml.cs: removed the static CrashLogPath field and the static
    LogCrash method; added a _crashLogger field. The three unhandled-exception
    handlers and the ShowTrayMenuPopup catch now delegate to it.

Testing

  • dotnet build clean (0 warnings, 0 errors).
  • Tray test suite green (872 tests).

Proof

Forced a crash at runtime (a temporary throw on a background thread, not part
of this change) to exercise the extracted logger through
OnDomainUnhandledException. crash.log received the expected entry:

Captura de pantalla 2026-05-31 011319 
[2026-05-31 01:12:38.464] DomainUnhandledException
System.InvalidOperationException: ProofCrash #554

(screenshot below)

Notes

Behavior is byte-for-byte identical: same timestamp format, same append mode,
same two-stage exception swallowing, same Logger.Error calls. No locking added
(the original had none). OnProcessExit / s_runMarker are untouched.

Addresses part of #554.

Co-Authored-By: Claude Sonnet 4.6 noreply@anthropic.com

AlexAlves87 @claude
refactor: extract AppCrashLogger from App.xaml.cs
d683cfd 
Move the crash-logging sink (LogCrash + CrashLogPath) out of App.xaml.cs
into a dedicated AppCrashLogger, mirroring the existing AppRunMarker. The
three unhandled-exception handlers and the ShowTrayMenuPopup catch now
delegate to the instance; behavior is byte-for-byte identical.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@clawsweeper

clawsweeper Bot commented May 30, 2026 •
edited
Loading

Copy link
Copy Markdown

Thanks for the context here. I swept through the related work, and this is now duplicate or superseded.

Keep this PR open: the branch is a focused cleanup that is not already on master, preserves the current crash-log behavior in source review, and includes real runtime proof of the extracted logger path.

Canonical path: Close this PR as superseded by #493.

So I’m closing this here and keeping the remaining discussion on #493.

Review details

Best possible solution:

Close this PR as superseded by #493.

Do we have a high-confidence way to reproduce the issue?

Not applicable. This is a refactor PR, and the contributor's attached runtime screenshot shows the changed crash-logging path still appends to crash.log after a forced DomainUnhandledException.

Is this the best way to solve the issue?

Yes. The extraction is narrow, mirrors the existing AppRunMarker service pattern, and avoids expanding the scope of the broader App/ConnectionPage decomposition issue.

Security review:

Security review cleared: No security or supply-chain concern found; the diff only moves existing local crash-log writing and logger calls into a dedicated service.

AGENTS.md: found and applied where relevant.

What I checked:

  • linked superseding PR: refactor: remove dead recording handler and extract two static helpers from App.xaml.cs #493 (refactor: remove dead recording handler and extract two static helpers from App.xaml.cs) is merged at 2026-05-21T19:56:32Z.
  • cluster evidence: the durable review links that PR in the work cluster or recommended risk path.
  • no human follow-up: live comments and timeline hydrated by apply contain no non-automation activity after the ClawSweeper review.

Likely related people:

  • AlexAlves87: Previously authored the merged AppRunMarker and CLI extraction work that this PR explicitly mirrors, so they are connected to this App.xaml.cs extraction pattern beyond only opening this PR. (role: recent refactor contributor; confidence: high; commits: 190a53fba49c; files: src/OpenClaw.Tray.WinUI/App.xaml.cs, src/OpenClaw.Tray.WinUI/Services/AppRunMarker.cs)
  • shanselman: Merged the earlier AppRunMarker extraction and authored the later P1: App.xaml.cs (4204 lines) and ConnectionPage.xaml.cs (2840 lines) are unmaintainable god files #554 hardening pass that intentionally left broader App decomposition as follow-up. (role: recent area contributor and merger; confidence: high; commits: 190a53fba49c, 0d4fcbd50ad5; files: src/OpenClaw.Tray.WinUI/App.xaml.cs, src/OpenClaw.Tray.WinUI/Services/AppRunMarker.cs)
  • ranjeshj: Recent file history for App.xaml.cs includes warning-clean and broader tray/App work, making them a plausible route for review of small App-surface refactors. (role: recent adjacent App.xaml.cs contributor; confidence: medium; commits: a727197dd709; files: src/OpenClaw.Tray.WinUI/App.xaml.cs)

Codex review notes: model gpt-5.5, reasoning high; reviewed against eb06fba21c44.

@clawsweeper clawsweeper Bot added proof: sufficient Contributor real behavior proof is sufficient. proof: 📸 screenshot Contributor real behavior proof includes screenshot evidence. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. P3 Low-risk cleanup, docs, polish, ergonomics, or speculative feature. labels May 30, 2026
@AlexAlves87

AlexAlves87 commented May 30, 2026

Copy link
Copy Markdown
Contributor Author

Thanks @clawsweeper for the thorough sweep — the security pass and the runtime-proof acknowledgement are much appreciated! 🙏

One small thing on the "superseded by #493" conclusion: I think there may be a mix-up there. #493 extracted AppRunMarker + CliUninstallHandler and removed the dead recording handler — it never touched the crash logger. CrashLogPath and the static LogCrash are still inline in App.xaml.cs on current master (around lines 200 and 317), which is exactly what this PR moves into AppCrashLogger.

So this one isn't a duplicate of #493 — they share the App.xaml.cs extraction pattern, but the code actually moved here isn't on master yet. The two PRs are independent slices.

@clawsweeper could you take another look when you get a chance? Happy to rebase or adjust if I'm missing something. 🙂

@AlexAlves87 AlexAlves87 mentioned this pull request May 31, 2026
Open
@ranjeshj
ranjeshj merged commit 779952e into openclaw:master Jun 2, 2026
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

P3 Low-risk cleanup, docs, polish, ergonomics, or speculative feature. proof: 📸 screenshot Contributor real behavior proof includes screenshot evidence. proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@AlexAlves87 @ranjeshj